Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(evm): added tx logs events to the funtoken related txs #2161

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

onikonychev
Copy link
Contributor

@onikonychev onikonychev commented Jan 10, 2025

Summary by CodeRabbit

  • Changelog

  • Testing Improvements

    • Enhanced transaction log testing across multiple components
    • Added mutex synchronization for concurrent transaction broadcasting
    • Improved transaction receipt handling and balance retrieval
  • Transaction Handling

    • Updated transaction broadcasting method to support optional account sequence
    • Modified Ethereum transaction transfer execution to return additional response information
  • EVM Functionality

    • Improved contract call logging and event emission
    • Updated log event handling in contract interactions

@onikonychev onikonychev requested a review from a team as a code owner January 10, 2025 18:03
Copy link
Contributor

coderabbitai bot commented Jan 10, 2025

Walkthrough

This pull request introduces enhancements to transaction log event handling and testing infrastructure across multiple components of the Nibiru blockchain. The changes focus on improving transaction traceability, particularly for fun token-related transactions, and updating test suites to support more robust transaction and log event testing. The modifications span several packages, including EVM, RPC backend, and common test utilities, with a consistent theme of improving transaction log emission and testing mechanisms.

Changes

File Change Summary
CHANGELOG.md Added entry for PR #2161 documenting fix for fun token transaction log events
eth/rpc/backend/backend_suite_test.go Added mutex for concurrent transaction testing, updated WaitForReceipt function signature
eth/rpc/backend/nonce_test.go Added mutex lock to prevent nonce conflicts, updated WaitForReceipt function call
eth/rpc/backend/tx_logs_test.go New test file with comprehensive transaction log testing suite
eth/rpc/backend/utils_test.go Updated test function with new log assertion and imports
x/common/testutil/testnetwork/tx.go Modified BroadcastMsgs method to support optional account sequence
x/common/testutil/testnetwork/tx_test.go Updated BroadcastMsgs method call with nil argument
x/evm/evmtest/tx.go Changed ExecuteNibiTransfer to return transaction response
x/evm/keeper/call_contract.go Reactivated log event emission with error handling
x/evm/keeper/grpc_query_test.go Updated transaction message retrieval
x/oracle/keeper/app_test.go Modified message broadcasting in test methods

Sequence Diagram

sequenceDiagram
    participant Client
    participant EVMBackend
    participant TransactionProcessor
    participant LogEmitter
    participant BlockIndexer

    Client->>EVMBackend: Send Transaction
    EVMBackend->>TransactionProcessor: Process Transaction
    TransactionProcessor->>LogEmitter: Emit Transaction Logs
    LogEmitter-->>TransactionProcessor: Log Emission Result
    TransactionProcessor->>BlockIndexer: Update Transaction Index
    BlockIndexer-->>TransactionProcessor: Indexing Confirmation
    TransactionProcessor-->>EVMBackend: Transaction Response
    EVMBackend-->>Client: Transaction Receipt
Loading

Possibly related PRs

Suggested reviewers

  • Unique-Divine
  • k-yang

Poem

🐰 Logs dancing through the chain,
Transactions hopping without strain,
Mutex guards our testing ground,
Fun tokens trace their merry round,
Code hops forward, clear and bright! 🚀

Finishing Touches

  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
eth/rpc/backend/nonce_test.go (1)

45-45: Consider handling the ignored receipt value.

The third return value from WaitForReceipt is being ignored. Consider either using it for additional verification or documenting why it's not needed.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ed351f and eab54cf.

📒 Files selected for processing (11)
  • CHANGELOG.md (1 hunks)
  • eth/rpc/backend/backend_suite_test.go (6 hunks)
  • eth/rpc/backend/nonce_test.go (2 hunks)
  • eth/rpc/backend/tx_logs_test.go (1 hunks)
  • eth/rpc/backend/utils_test.go (2 hunks)
  • x/common/testutil/testnetwork/tx.go (2 hunks)
  • x/common/testutil/testnetwork/tx_test.go (1 hunks)
  • x/evm/evmtest/tx.go (2 hunks)
  • x/evm/keeper/call_contract.go (1 hunks)
  • x/evm/keeper/grpc_query_test.go (2 hunks)
  • x/oracle/keeper/app_test.go (2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
eth/rpc/backend/backend_suite_test.go

[error] 198-198: func (*BackendSuite).getUnibiBalance is unused

(unused)

🪛 GitHub Check: lint
eth/rpc/backend/backend_suite_test.go

[failure] 198-198:
func (*BackendSuite).getUnibiBalance is unused (unused)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: integration-tests
🔇 Additional comments (20)
eth/rpc/backend/utils_test.go (2)

7-8: LGTM! Well-structured imports.

The addition of Ethereum's standard libraries for handling common types, core types, and crypto operations is appropriate for implementing proper transaction log verification.

Also applies to: 11-11


24-33: Comprehensive test coverage for Transfer event logs.

The test now properly verifies the structure of transaction logs, including:

  • Contract address verification
  • Event signature verification using Keccak256
  • Topic verification for Transfer events
x/common/testutil/testnetwork/tx_test.go (1)

19-19: LGTM! Proper usage of the new API.

The test correctly uses the new BroadcastMsgs API by passing nil for accountSequence, allowing the function to use the default sequence from the account.

eth/rpc/backend/nonce_test.go (1)

16-18: LGTM! Thread-safe test implementation.

Good practice to use mutex for preventing nonce conflicts during concurrent transaction broadcasting.

x/common/testutil/testnetwork/tx.go (3)

129-131: LGTM! Enhanced API flexibility.

Good addition of the optional accountSequence parameter, allowing better control over transaction sequencing while maintaining backward compatibility.


153-158: LGTM! Clean sequence handling implementation.

The implementation properly handles both custom and default sequence values, making it flexible for various use cases.


147-147: Verify the increased gas limit.

The gas limit has been increased by 10x. Please verify that this increase is necessary and document the reason for this change.

✅ Verification successful

Test utility gas limit increase is reasonable

The increased gas limit (10M) is specifically for the test network utility and doesn't affect production code. This is a reasonable change as:

  • It's isolated to test environment only
  • Test scenarios often require higher gas limits to accommodate various test cases
  • The production SDK broadcast still uses a lower 2M gas limit
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for gas-related issues or discussions in the codebase
rg -A 3 "gas.*limit.*TO_MICRO|insufficient.*gas" 

Length of output: 1041


Script:

#!/bin/bash
# Search for TO_MICRO constant definition and usage
echo "=== TO_MICRO constant ==="
rg -A 2 "TO_MICRO.*=|const.*TO_MICRO"

echo -e "\n=== Gas limit configurations ==="
rg -A 2 "SetGasLimit.*TO_MICRO|gasLimit.*TO_MICRO"

Length of output: 826


Script:

#!/bin/bash
# Search for gas-related test failures and discussions
echo "=== Gas-related test patterns ==="
rg -A 3 "gas.*test.*fail|test.*gas.*fail"

echo -e "\n=== Gas consumption patterns in tests ==="
rg -A 3 "GasConsumed|gas.*consumed" "**/*_test.go"

Length of output: 325


Script:

#!/bin/bash
# Search for gas consumption and configurations in tests
echo "=== Gas consumption and configurations in tests ==="
rg -A 3 "GasConsumed|gas.*consumed|gas.*config" --type go --type-add 'go:*.go'

echo -e "\n=== Gas-related errors in tests ==="
rg -A 3 "gas.*error|insufficient.*gas|out.*of.*gas" --type go

Length of output: 31232

x/evm/keeper/call_contract.go (2)

141-144: LGTM! Error handling for log emission is properly implemented.

The addition of log event emission with proper error handling enhances transaction traceability.


145-146: Verify transaction index update sequence.

The block transaction index is correctly updated after successful log emission.

Run the following script to verify the transaction index update sequence:

✅ Verification successful

Transaction index update sequence verified

The block transaction index is consistently updated after log emissions in both call_contract.go and msg_server.go, following the correct sequence:

  1. Emit logs/events
  2. Calculate new index
  3. Update block transaction index
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that transaction index updates follow log emissions.
# Test: Search for other places where block transaction index is updated.
rg -A 5 'BlockTxIndex.Set'

Length of output: 501


Script:

#!/bin/bash
# Get context around BlockTxIndex.Set calls
ast-grep --pattern 'k.EvmState.BlockTxIndex.Set($$$)'

# Look for log emission patterns
rg -B 10 'BlockTxIndex.Set' 

Length of output: 1649

x/oracle/keeper/app_test.go (2)

120-123: LGTM! Updated BroadcastMsgs call signature.

The prevote broadcast correctly includes the nil account sequence parameter.


Line range hint 136-141: LGTM! Updated BroadcastMsgs call signature.

The vote broadcast correctly includes the nil account sequence parameter.

eth/rpc/backend/backend_suite_test.go (2)

37-38: LGTM! Added mutex for test synchronization.

The mutex prevents nonce conflicts during concurrent transaction broadcasts.


174-190: LGTM! Enhanced WaitForReceipt with transaction receipt.

The function now returns additional transaction receipt information, improving test capabilities.

eth/rpc/backend/tx_logs_test.go (4)

21-29: LGTM! Well-documented test scenarios.

The test cases comprehensively cover different transaction types and their expected log events.


31-33: LGTM! Proper test synchronization.

The mutex is correctly used to prevent nonce conflicts during transaction broadcasts.


221-259: LGTM! Robust assertion helper.

The assertTxLogsAndTxIndex function thoroughly validates both transaction logs and indices.


261-288: LGTM! Comprehensive log matching.

The assertTxLogsMatch function performs detailed validation of log contents.

x/evm/evmtest/tx.go (1)

29-29: LGTM! Enhanced function signature to include transaction response.

The change to return both the transaction message and response provides better visibility into transaction execution results, which is particularly useful for tracking transaction logs and events.

Also applies to: 46-46

x/evm/keeper/grpc_query_test.go (1)

770-770: LGTM! Tests updated to handle new return value.

Test code correctly adapted to handle the new function signature by properly capturing both return values, even though the response isn't needed in these test cases.

Also applies to: 846-846

CHANGELOG.md (1)

78-78: LGTM! Clear and well-placed changelog entry.

The changelog entry accurately describes the changes made and is properly placed in the Nibiru EVM section.

eth/rpc/backend/backend_suite_test.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.

Project coverage is 65.14%. Comparing base (8db476f) to head (b5d5595).

Files with missing lines Patch % Lines
x/common/testutil/testnetwork/tx.go 62.50% 1 Missing and 2 partials ⚠️
x/evm/keeper/call_contract.go 25.00% 2 Missing and 1 partial ⚠️
x/evm/evmtest/tx.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2161      +/-   ##
==========================================
+ Coverage   65.12%   65.14%   +0.01%     
==========================================
  Files         277      277              
  Lines       22267    22278      +11     
==========================================
+ Hits        14502    14513      +11     
+ Misses       6771     6770       -1     
- Partials      994      995       +1     
Files with missing lines Coverage Δ
x/evm/evmtest/tx.go 55.32% <0.00%> (ø)
x/common/testutil/testnetwork/tx.go 65.45% <62.50%> (-0.90%) ⬇️
x/evm/keeper/call_contract.go 81.33% <25.00%> (-2.96%) ⬇️

... and 2 files with indirect coverage changes

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
eth/rpc/backend/backend_suite_test.go (1)

238-238: Remove commented-out code.

Commented-out code should be removed rather than left in the codebase. If this line is needed for future reference, consider documenting it in a more appropriate place.

-//recipient := crypto.CreateAddress(s.fundedAccEthAddr, 29381)
eth/rpc/backend/tx_logs_test.go (1)

31-220: Consider using table-driven tests for better maintainability.

While the test implementation is thorough and covers all scenarios, consider restructuring it as a table-driven test. This would make it easier to add new test cases and maintain the existing ones.

Example structure:

func (s *BackendSuite) TestLogs() {
    testCases := []struct {
        name        string
        setup       func() (txHash gethcommon.Hash)
        expectLogs  []*gethcore.Log
        expectEthTx bool
    }{
        {
            name: "simple eth transfer",
            setup: func() gethcommon.Hash {
                randomEthAddr := evmtest.NewEthPrivAcc().EthAddr
                return s.SendNibiViaEthTransfer(randomEthAddr, amountToSend, false)
            },
            expectLogs:  nil,
            expectEthTx: true,
        },
        // Add other test cases...
    }
    
    for _, tc := range testCases {
        s.Run(tc.name, func() {
            // Test implementation
        })
    }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eab54cf and 7a49a1b.

📒 Files selected for processing (3)
  • eth/rpc/backend/backend_suite_test.go (7 hunks)
  • eth/rpc/backend/tx_logs_test.go (1 hunks)
  • eth/rpc/backend/utils_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • eth/rpc/backend/utils_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: integration-tests
🔇 Additional comments (5)
eth/rpc/backend/backend_suite_test.go (3)

37-38: LGTM! Good synchronization practice.

The addition of a global mutex for synchronizing concurrent transaction broadcasts is a good practice to prevent race conditions in tests.


174-190: LGTM! Enhanced receipt handling.

The function now properly returns the transaction receipt along with block number and hash, with consistent error handling and timeout behavior.


199-202: LGTM! Good simplification.

The function has been simplified while maintaining its functionality.

eth/rpc/backend/tx_logs_test.go (2)

22-34: LGTM! Well-documented test setup.

The test documentation clearly outlines the test scenarios, and the mutex is properly used to prevent nonce conflicts during transaction broadcasting.


222-289: LGTM! Well-structured helper functions.

The helper functions are well-implemented with:

  • Comprehensive assertions
  • Descriptive error messages
  • Clear separation of concerns

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
eth/rpc/backend/tx_logs_test.go (3)

22-37: Consider adding timeout to prevent test deadlock.

While the mutex handling is correct, consider adding a timeout mechanism to prevent potential deadlocks in case of test failures.

 testMutex.Lock()
-defer testMutex.Unlock()
+defer func() {
+    // Ensure mutex is always unlocked, even if test panics
+    testMutex.Unlock()
+}()
+
+// Add timeout context to prevent test hanging
+ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+defer cancel()

39-120: Enhance error messages for better debugging.

While the error handling is thorough, consider enhancing error messages to include more context about the transaction state.

-s.Require().Equal(
-    uint32(0),
-    txResp.Code,
-    fmt.Sprintf("Failed to create FunToken from ERC20. RawLog: %s", txResp.RawLog),
-)
+s.Require().Equal(
+    uint32(0),
+    txResp.Code,
+    fmt.Sprintf(
+        "Failed to create FunToken from ERC20. Code: %d, RawLog: %s, GasUsed: %d",
+        txResp.Code,
+        txResp.RawLog,
+        txResp.GasUsed,
+    ),
+)

266-293: Consider adding type safety checks for log data.

The log comparison could be more robust with additional type safety checks for the log data.

 func (s *BackendSuite) assertTxLogsMatch(
     expectedLogs []*gethcore.Log,
     actualLogs []*gethcore.Log,
     txInfo string,
 ) {
+    if expectedLogs == nil || actualLogs == nil {
+        s.Fail("Nil logs provided for comparison")
+        return
+    }
+
     for idx, expectedLog := range expectedLogs {
         actualLog := actualLogs[idx]
+        if expectedLog == nil || actualLog == nil {
+            s.Fail("Nil log entry encountered")
+            return
+        }
+
         s.Require().Equal(
             expectedLog.Address,
             actualLog.Address,
             fmt.Sprintf("log contract address mismatch, log index %d, %s", idx, txInfo),
         )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5977b7a and ef106f5.

📒 Files selected for processing (1)
  • eth/rpc/backend/tx_logs_test.go (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: integration-tests
🔇 Additional comments (2)
eth/rpc/backend/tx_logs_test.go (2)

1-20: LGTM! Well-organized imports.

The package declaration and import organization follow good practices, clearly separating external and internal dependencies.


1-293: Overall excellent test implementation!

The test suite is well-structured and comprehensive, covering various transaction types and their log events. The implementation includes proper error handling, mutex synchronization, and thorough log verification. While there are some suggested improvements for robustness, the current implementation is solid and achieves its testing objectives.

Comment on lines 200 to 219
// Getting block results. Note: txs could be included in more than one block
blockNumber := blockNumFirstTx.Int64()
blockRes, err := s.backend.TendermintBlockResultByNumber(&blockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockRes)
txIndex := 0
for idx, check := range checks {
if txIndex+1 > len(blockRes.TxsResults) {
blockNumber++
if blockNumber > blockNumLastTx.Int64() {
s.Fail("TX %d not found in block results", idx)
}
txIndex = 0
blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockRes)
}
s.assertTxLogsAndTxIndex(blockRes, txIndex, check.logs, check.expectEthTx, check.txInfo)
txIndex++
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add bounds checking for block results.

The block result iteration could be more robust with additional bounds checking.

 if txIndex+1 > len(blockRes.TxsResults) {
     blockNumber++
     if blockNumber > blockNumLastTx.Int64() {
-        s.Fail("TX %d not found in block results", idx)
+        s.Fail(
+            "Transaction %d (%s) not found in blocks %d to %d",
+            idx,
+            check.txInfo,
+            blockNumFirstTx.Int64(),
+            blockNumLastTx.Int64(),
+        )
     }
     txIndex = 0
+    // Add maximum block search limit
+    if blockNumber - blockNumFirstTx.Int64() > 10 {
+        s.Fail("Exceeded maximum block search range")
+    }
     blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber)
     s.Require().NoError(err)
     s.Require().NotNil(blockRes)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Getting block results. Note: txs could be included in more than one block
blockNumber := blockNumFirstTx.Int64()
blockRes, err := s.backend.TendermintBlockResultByNumber(&blockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockRes)
txIndex := 0
for idx, check := range checks {
if txIndex+1 > len(blockRes.TxsResults) {
blockNumber++
if blockNumber > blockNumLastTx.Int64() {
s.Fail("TX %d not found in block results", idx)
}
txIndex = 0
blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockRes)
}
s.assertTxLogsAndTxIndex(blockRes, txIndex, check.logs, check.expectEthTx, check.txInfo)
txIndex++
}
// Getting block results. Note: txs could be included in more than one block
blockNumber := blockNumFirstTx.Int64()
blockRes, err := s.backend.TendermintBlockResultByNumber(&blockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockRes)
txIndex := 0
for idx, check := range checks {
if txIndex+1 > len(blockRes.TxsResults) {
blockNumber++
if blockNumber > blockNumLastTx.Int64() {
s.Fail(
"Transaction %d (%s) not found in blocks %d to %d",
idx,
check.txInfo,
blockNumFirstTx.Int64(),
blockNumLastTx.Int64(),
)
}
txIndex = 0
// Add maximum block search limit
if blockNumber - blockNumFirstTx.Int64() > 10 {
s.Fail("Exceeded maximum block search range")
}
blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockRes)
}
s.assertTxLogsAndTxIndex(blockRes, txIndex, check.logs, check.expectEthTx, check.txInfo)
txIndex++
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
eth/rpc/backend/tx_logs_test.go (5)

22-31: Consider enhancing test documentation with expected log events.

While the documentation clearly outlines the test steps, it would be more comprehensive to include the expected log events for each transaction type. This would make the test's expectations more explicit and aid in maintenance.


32-38: Consider adding test cleanup.

While the test setup is good with mutex locking and fresh block creation, consider adding cleanup steps after test execution to reset the state. This would ensure test isolation and prevent potential side effects in subsequent tests.


103-113: Consider making gas parameters configurable.

The hardcoded gas values (Gas: 1_500_000) could be problematic if gas requirements change. Consider making these configurable or calculating them dynamically based on the operation.


121-198: Consider validating log data more extensively.

While the test validates log topics and addresses, it could be more thorough by also validating:

  • Log data field contents
  • Event parameter values
  • Block and transaction hashes

272-299: Enhance log validation in helper functions.

Consider adding validation for:

  1. Null checks for log parameters
  2. Log ordering verification
  3. Data field length validation

This would make the test more robust against edge cases.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef106f5 and b5d5595.

📒 Files selected for processing (2)
  • eth/rpc/backend/tx_logs_test.go (1 hunks)
  • x/evm/keeper/call_contract.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • x/evm/keeper/call_contract.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: integration-tests
🔇 Additional comments (2)
eth/rpc/backend/tx_logs_test.go (2)

1-20: LGTM! Well-organized imports.

The imports are properly organized and all dependencies are necessary for the test implementation.


208-218: Add bounds checking for block results.

The block result iteration could be more robust with additional bounds checking.

Comment on lines +115 to +120
// Wait for all txs to be included in a block
blockNumFirstTx, _, _ := WaitForReceipt(s, txHashFirst)
blockNumLastTx, _, _ := WaitForReceipt(s, txHashLast)
s.Require().NotNil(blockNumFirstTx)
s.Require().NotNil(blockNumLastTx)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add timeout handling for transaction receipt waiting.

The WaitForReceipt calls could potentially hang if transactions are never mined. Consider adding a timeout mechanism to fail fast in such scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants